[STORE] abstract snapshot catalog and add Redis snapshot backend - #1739

This TrimAsciiWhitespace function is inefficient because value.erase(value.begin()) has linear time complexity and is being called inside a loop. This can be optimized to have linear complexity overall by finding the first and last non-whitespace characters and creating a substring.

Additionally, this function is duplicated in mooncake-store/src/ha/serializer_snapshot_store.cpp. It would be best to move this to a shared utility file to avoid code duplication.

Here's a more efficient implementation. Since the function name is TrimAsciiWhitespace, using an explicit list of ASCII whitespace characters is more correct than the locale-dependent isspace().

std::string TrimAsciiWhitespace(std::string value) {
    const auto first = value.find_first_not_of(" \t\n\r\f\v");
    if (std::string::npos == first) {
        return "";
    }
    const auto last = value.find_last_not_of(" \t\n\r\f\v");
    return value.substr(first, (last - first + 1));
}

I moved the shared snapshot parsing/building helpers into a common snapshot helper so both serializer- and Redis-backed snapshot stores now use the same implementation. I also switched the trim logic to a linear first/last non-whitespace scan.

The lazy initialization logic here seems redundant and could be confusing. The MasterService constructor already initializes snapshot_store_ via CreateSnapshotStore() right after snapshot_backend_ is created (inside the if (enable_snapshot_ || enable_snapshot_restore_) block).

Therefore, the condition !snapshot_store_ && snapshot_backend_ should never be true, making the lazy initialization appear to be dead code.

To improve clarity and remove the redundancy, you could simplify this method to just return snapshot_store_.get(). If there's a scenario where this lazy initialization is needed that I'm missing, it would be helpful to add a comment explaining it.

ha::SnapshotStore* MasterService::GetSnapshotStore() {
    return snapshot_store_.get();
}

I kept the eager constructor path for the normal case, but changed GetSnapshotStore() so the fallback path is synchronized and only materializes the store when snapshot_backend_ is injected after construction. That preserves the late-bound test path without leaving an unsynchronized lazy-init branch.

ConnectRedisForTest and BuildRedisMasterViewKey were moved into mooncake::testing in redis_test_utils.h, but this call site still uses unqualified names inside namespace mooncake. This will fail to compile unless there’s a local using declaration not shown in the diff. Prefer qualifying the calls (e.g., testing::ConnectRedisForTest, testing::BuildRedisMasterViewKey) or adding explicit using mooncake::testing::...; in the local anonymous namespace.

redis_snapshot_store_test.cpp defines its own main(), but the target links gtest_main, which also provides main(). This typically causes a duplicate symbol linker error. Fix by removing gtest_main from target_link_libraries(...) for this target, or removing the custom main() from the test source and letting gtest_main handle it (if you still need gflags parsing, keep the custom main() and drop gtest_main).

Fixed in the latest update: I dropped gtest_main from the redis_snapshot_store_test target since the test already provides its own main().

GetSnapshotStore() lazily initializes snapshot_store_ without synchronization, which can race if multiple threads call it concurrently (writes to a std::unique_ptr are not thread-safe). A concrete fix is to guard initialization with a mutex/shared_mutex (you already have snapshot_mutex_, if appropriate), or use std::once_flag + std::call_once. Alternatively, since the constructor already sets snapshot_store_ = CreateSnapshotStore();, consider removing the lazy-init path entirely to avoid split initialization logic.

I updated GetSnapshotStore() so the fallback materialization path is synchronized. The normal path is still eager construction, and the lazy path now only exists for the late-bound backend case used by tests.

Delete() updates the Redis catalog first and deletes payload objects second; if payload deletion fails, the catalog no longer references the payload, leaving orphaned snapshot data and making automated cleanup harder. Consider reordering (delete payload first, then update Redis) or implementing a compensating action on failure (e.g., re-add the snapshot to the ZSET and re-set latest if needed), acknowledging the tradeoff that cross-backend atomicity isn’t possible but inconsistency can be minimized.

If payload deletion fails afterwards, we may leave orphaned payload, but reversing the order risks leaving the catalog/latest marker pointing to missing payload, which is the worse user-visible inconsistency. A follow-up GC/compensation path would be a better improvement here.

Need check if the snapshot_id is latest

SerializerSnapshotStore::Delete() now checks whether the removed snapshot is the current latest marker, advances it to the next snapshot when available, and clears the latest marker when deleting the last snapshot.

check of snapshot_backend_ here is strange. may put it somewhere else

I tightened this path in the latest update. The normal case still uses constructor-time initialization, and GetSnapshotStore() now only falls back to synchronized late materialization when snapshot_backend_ is injected after construction. That lets us remove the old split/unsynchronized behavior while keeping the late-bound test path working.

Now CreateSnapshotStore is called in MasterService, why check it here

This func is called in every api， should we make it a long connect?

Snapshot catalog operations are not on a hot path today, and keeping per-call connections avoids introducing shared hiredis context / reconnect / threading complexity prematurely. If catalog operations become measurable in practice, connection reuse would be a reasonable follow-up.

make it a common function

The duplicated snapshot helper logic is now shared instead of being implemented independently in both snapshot store backends.

make the 0 a macro

Replaced the List(0) callsite with a named unlimited-list constant for readability.

why there are two localtions for the serialize related api

The split is intentional here: SerializerBackend is the payload/object-store primitive layer, while SnapshotStore is the snapshot catalog semantics layer. This PR only abstracts the catalog side, so I’d prefer to keep that layering explicit instead of collapsing the two concerns in the same change.

this is only used for serial? maybe make it more abstract

In this PR I kept snapshot_backend_type as the payload-backend knob and snapshot_catalog_backend_type as the catalog-backend knob. That seemed like the smallest change that makes the split explicit without expanding the scope into a config rename/migration.

Both leader election and snapshot management can be offloaded to Redis. maybe isolate the snapshot directory from the leader election mechanism to decouple these two distinct responsibilities.

The shared Redis helper here is only the low-level transport/auth/endpoint utility. The actual semantics remain separated in the leader-coordinator and snapshot-store implementations. I kept the helper shared in this PR to avoid duplicating the same connection logic in two places.

same above. this check looks unnecessary.